fix(hindsight): add local_embedded mode and full local-mode fields to config schema - #84586
blut-agent wants to merge 6 commits into
Conversation
… config schema The desktop Memory & context panel was only showing 'cloud' and 'local_external' modes, so Hindsight instances in 'local_embedded' mode displayed incorrect cloud defaults (wrong API URL, missing LLM fields, wrong API key env var). - Add 'local_embedded' to the mode select options - Add all local-mode fields: llm_provider, llm_base_url, llm_model, llm_api_key, idle_timeout, port_health_grace_timeout - Add shared fields from the wizard schema - Add conditional visibility via the 'when' attribute on fields so local-mode fields only appear when mode=local_embedded - Add _read_field_dep() helper and conditional visibility filter in _declared_provider_payload() - Add 'when' attribute to ProviderField dataclass Fixes NousResearch#84572.
alt-glitch
added
type/bug
aikeepsitreal
left a comment
aikeepsitreal
left a comment
There was a problem hiding this comment.
Thanks for tackling #84572. I traced this patch through the declared-schema GET/PUT path and the current Desktop autosave renderer. There are several correctness issues to address before this is safe to merge:
-
Fresh/default configs lose their cloud fields.
_read_field_dep()only reads stored data. Whenmodeis absent, the Mode field correctly renders its declared default (cloud), but everywhen={"mode": "cloud"}field seesNoneand is omitted. This regresses the existing safe-default contract: a fresh Hindsight panel would show Cloud without its URL/key fields. Predicate resolution needs to use the dependency field's declared default (and its aliases/env fallbacks), not raw storage alone. -
Multi-key predicates are not evaluated. Both helpers use
next(iter(...)), sowhen={"mode": "local_embedded", "llm_provider": "openai_compatible"}checks onlymode. The endpoint field will therefore appear for every embedded provider. All predicate pairs need to be checked. -
Server-side omission does not react to the current Desktop autosave flow. The inline Mode select saves one key without refreshing
config.fields(the panel intentionally avoids a refresh to preserve sibling drafts). Switching Cloud ↔ Local External leaves the old mode's controls rendered until the panel is reopened/reloaded. Either the renderer needs the condition metadata and must evaluate it against current drafts, or dependency commits need a carefully designed refresh/reseed path. -
This crosses the intentional setup boundary. Commit
03d9a95a74and the current Hindsight schema test explicitly keeplocal_embeddedunselectable in Desktop because setup owns dependency installation. The declared PUT path only writes config and activates the provider; it does not installhindsight-all. Making this option selectable can create a configured-but-unavailable provider. Existing embedded values should be displayed honestly, but selecting the mode should remain setup-owned unless this is routed through the setup workflow. -
The schema/test contract is currently broken. The existing Hindsight schema tests assert the exact field set, that all fields are inline, and that
local_embeddedis not writable; this PR does not update or replace them with behavior coverage. It also declares duplicateapi_url/api_keykeys even though the generic renderer and seed maps assume stable unique field keys. -
Two smaller drift/lint issues:
timeoutis declared as 30 while the runtime/wizard default is_DEFAULT_TIMEOUT = 120;KIND_JSONis imported but unused, which Ruff should reject.
A narrower approach would preserve an unsupported stored select value as a disabled/read-only option, add the four embedded LLM fields to Full config, keep the Mode write allow-list at Cloud/Local External, and cover the actual declared GET/PUT plus Desktop select behavior. That fixes the misleading display without adding a generic condition system or bypassing setup.
|
I completed and validated the narrower implementation described in my review. Since this connected account cannot push to the upstream repository or the PR head fork, here is the exact apply-ready patch so the working fix is not blocked on my local checkout. Validation:
Apply-ready patchdiff --git a/apps/desktop/src/app/settings/memory/field-control.tsx b/apps/desktop/src/app/settings/memory/field-control.tsx
index d16a605718..57493020ca 100644
--- a/apps/desktop/src/app/settings/memory/field-control.tsx
+++ b/apps/desktop/src/app/settings/memory/field-control.tsx
@@ -87,7 +87,7 @@ export function FieldControl({
</SelectTrigger>
<SelectContent>
{field.options.map(option => (
- <SelectItem key={option.value} value={option.value}>
+ <SelectItem disabled={option.disabled} key={option.value} value={option.value}>
{option.label}
</SelectItem>
))}
diff --git a/apps/desktop/src/app/settings/memory/provider-config-panel.test.tsx b/apps/desktop/src/app/settings/memory/provider-config-panel.test.tsx
index ac4fe9c502..1c084d20d2 100644
--- a/apps/desktop/src/app/settings/memory/provider-config-panel.test.tsx
+++ b/apps/desktop/src/app/settings/memory/provider-config-panel.test.tsx
@@ -99,6 +99,7 @@ function honchoSchema(): MemoryProviderConfig {
}
beforeEach(() => {
+ Element.prototype.scrollIntoView = vi.fn()
getMemoryProviderConfig.mockResolvedValue(honchoSchema())
saveMemoryProviderConfig.mockResolvedValue({ ok: true })
})
@@ -186,6 +187,32 @@ describe('ProviderConfigPanel', () => {
expect(screen.getByRole('button', { name: /Full config/ })).toBeTruthy()
})
+ it('shows a stored select value that Desktop cannot choose', async () => {
+ const config = honchoSchema()
+ const environment = config.fields.find(field => field.key === 'environment')
+
+ if (!environment) {
+ throw new Error('environment field missing from fixture')
+ }
+
+ environment.value = 'local_embedded'
+ environment.options.push({
+ value: 'local_embedded',
+ label: 'Local Embedded',
+ description: 'This stored value is not selectable in Desktop.',
+ disabled: true
+ })
+
+ getMemoryProviderConfig.mockResolvedValue(config)
+
+ await renderPanel()
+
+ expect(await screen.findByText('Local Embedded')).toBeTruthy()
+ fireEvent.click(screen.getByRole('combobox'))
+ const option = await screen.findByRole('option', { name: 'Local Embedded' })
+ expect(option.getAttribute('aria-disabled')).toBe('true')
+ })
+
it('shows an inline error with retry when the load fails, then recovers', async () => {
getMemoryProviderConfig.mockRejectedValueOnce(new Error('Timed out connecting to Hermes backend'))
diff --git a/apps/desktop/src/types/hermes.ts b/apps/desktop/src/types/hermes.ts
index 669813b0ab..49be5602b2 100644
--- a/apps/desktop/src/types/hermes.ts
+++ b/apps/desktop/src/types/hermes.ts
@@ -124,6 +124,7 @@ export type MemoryProviderFieldKind = 'bool' | 'json' | 'number' | 'secret' | 's
export interface MemoryProviderFieldOption {
description: string
+ disabled?: boolean
label: string
value: string
}
diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py
index d9b7a1a56b..59b8c7b643 100644
--- a/hermes_cli/web_server.py
+++ b/hermes_cli/web_server.py
@@ -5367,6 +5367,24 @@ def _provider_field_entry(field: ProviderField) -> Dict[str, Any]:
}
+def _stored_select_option(value: str) -> Dict[str, Any]:
+ """Represent a stored value that this surface deliberately cannot select.
+
+ Older versions, another Hermes surface, or a hand-edited provider config
+ can contain a valid runtime value outside the Desktop declaration. Showing
+ that value honestly is read-only: the declared options still remain the
+ write allow-list enforced by ``_coerce_field_value``.
+ """
+
+ label = value.replace("_", " ").replace("-", " ").title()
+ return {
+ "value": value,
+ "label": label,
+ "description": "This stored value is not selectable in Desktop.",
+ "disabled": True,
+ }
+
+
# Sentinel: remove this key so it falls back to the host or built-in default.
_UNSET: Any = object()
@@ -5547,7 +5565,10 @@ def _declared_provider_payload(provider: ProviderConfigSchema) -> Dict[str, Any]
value = _serialize_field_value(field, native)
if field.kind == "select" and value not in field.allowed_values():
- value = field.default
+ if native is None:
+ value = field.default
+ else:
+ entry["options"].append(_stored_select_option(value))
entry["value"] = value
# Presence, not truthiness — a stored False/0 is still "set".
entry["is_set"] = native is not None if is_honcho else bool(value)
diff --git a/plugins/memory/hindsight/config_schema.py b/plugins/memory/hindsight/config_schema.py
index a6e7ef9ee7..671410655b 100644
--- a/plugins/memory/hindsight/config_schema.py
+++ b/plugins/memory/hindsight/config_schema.py
@@ -13,6 +13,7 @@ CONFIG_SCHEMA = ProviderConfigSchema(
name="hindsight",
label="Hindsight",
fields=(
+ # — Connection —
ProviderField(
key="mode",
label="Mode",
@@ -32,25 +33,76 @@ CONFIG_SCHEMA = ProviderConfigSchema(
),
),
inline=True,
+ group="Connection",
),
ProviderField(
key="api_key",
- label="API key",
+ label="Hindsight API key",
kind=KIND_SECRET,
env_key="HINDSIGHT_API_KEY",
- description="Used to authenticate with the Hindsight API.",
+ description="Cloud or Local External authentication. Not used by Local Embedded mode.",
placeholder="Enter Hindsight API key",
inline=True,
+ group="Connection",
),
ProviderField(
key="api_url",
- label="API URL",
+ label="Hindsight API URL",
kind=KIND_TEXT,
default="https://api.hindsight.vectorize.io",
aliases=("apiUrl",),
env_fallbacks=("HINDSIGHT_API_URL",),
+ description="Cloud or Local External endpoint. Not used by Local Embedded mode.",
inline=True,
+ group="Connection",
),
+ # — Local Embedded LLM —
+ ProviderField(
+ key="llm_provider",
+ label="LLM provider",
+ kind=KIND_SELECT,
+ default="openai",
+ description="LLM backend used by Local Embedded mode.",
+ options=(
+ ProviderFieldOption("openai", "OpenAI"),
+ ProviderFieldOption("anthropic", "Anthropic"),
+ ProviderFieldOption("gemini", "Gemini"),
+ ProviderFieldOption("groq", "Groq"),
+ ProviderFieldOption("openrouter", "OpenRouter"),
+ ProviderFieldOption("minimax", "MiniMax"),
+ ProviderFieldOption("ollama", "Ollama"),
+ ProviderFieldOption("lmstudio", "LM Studio"),
+ ProviderFieldOption("openai_compatible", "OpenAI compatible"),
+ ),
+ group="Local Embedded LLM",
+ ),
+ ProviderField(
+ key="llm_base_url",
+ label="LLM base URL",
+ kind=KIND_TEXT,
+ description="Custom LLM endpoint used by Local Embedded mode.",
+ placeholder="https://…/v1",
+ group="Local Embedded LLM",
+ ),
+ ProviderField(
+ key="llm_api_key",
+ label="LLM API key",
+ kind=KIND_SECRET,
+ env_key="HINDSIGHT_LLM_API_KEY",
+ description="LLM credential used by Local Embedded mode. Optional for some local servers.",
+ placeholder="Enter LLM API key",
+ group="Local Embedded LLM",
+ ),
+ ProviderField(
+ key="llm_model",
+ label="LLM model",
+ kind=KIND_TEXT,
+ default="gpt-4o-mini",
+ description="Model used by Local Embedded mode.",
+ placeholder="gpt-4o-mini",
+ group="Local Embedded LLM",
+ ),
+ # — Memory —
ProviderField(
key="bank_id",
label="Bank ID",
@@ -58,6 +110,7 @@ CONFIG_SCHEMA = ProviderConfigSchema(
default="hermes",
aliases=("bankId",),
inline=True,
+ group="Memory",
),
ProviderField(
key="recall_budget",
@@ -71,6 +124,7 @@ CONFIG_SCHEMA = ProviderConfigSchema(
ProviderFieldOption("high", "high"),
),
inline=True,
+ group="Memory",
),
),
)
diff --git a/tests/hermes_cli/test_web_server.py b/tests/hermes_cli/test_web_server.py
index 895b0ddf79..95546160b4 100644
--- a/tests/hermes_cli/test_web_server.py
+++ b/tests/hermes_cli/test_web_server.py
@@ -684,9 +684,58 @@ class TestWebServerEndpoints:
assert resp.status_code == 200
data = resp.json()
fields = self._provider_field_map(data)
- assert set(fields) == {"mode", "api_key", "api_url", "bank_id", "recall_budget"}
+ assert {
+ "mode",
+ "api_key",
+ "api_url",
+ "llm_provider",
+ "llm_base_url",
+ "llm_api_key",
+ "llm_model",
+ "bank_id",
+ "recall_budget",
+ } <= set(fields)
assert fields["mode"]["kind"] == "select"
assert fields["api_key"]["kind"] == "secret"
+ assert fields["llm_api_key"]["kind"] == "secret"
+
+ def test_declared_surface_preserves_local_embedded_values(self):
+ from hermes_constants import get_hermes_home
+ from hermes_cli.config import save_env_value
+
+ config_path = get_hermes_home() / "hindsight" / "config.json"
+ config_path.parent.mkdir(parents=True, exist_ok=True)
+ config_path.write_text(
+ json.dumps(
+ {
+ "mode": "local_embedded",
+ "llm_provider": "openai_compatible",
+ "llm_base_url": "https://llm.example/v1",
+ "llm_model": "example-model",
+ }
+ ),
+ encoding="utf-8",
+ )
+ save_env_value("HINDSIGHT_LLM_API_KEY", "local-secret")
+
+ resp = self.client.get("/api/memory/providers/hindsight/config?surface=declared")
+
+ assert resp.status_code == 200
+ data = resp.json()
+ fields = self._provider_field_map(data)
+ assert fields["mode"]["value"] == "local_embedded"
+ local_option = next(
+ option
+ for option in fields["mode"]["options"]
+ if option["value"] == "local_embedded"
+ )
+ assert local_option["disabled"] is True
+ assert fields["llm_provider"]["value"] == "openai_compatible"
+ assert fields["llm_base_url"]["value"] == "https://llm.example/v1"
+ assert fields["llm_model"]["value"] == "example-model"
+ assert fields["llm_api_key"]["is_set"] is True
+ assert fields["llm_api_key"]["value"] == ""
+ assert "local-secret" not in json.dumps(data)
def test_declared_surface_hides_undeclared_providers(self):
resp = self.client.get("/api/memory/providers/builtin/config?surface=declared")
@@ -698,26 +747,42 @@ class TestWebServerEndpoints:
from hermes_constants import get_hermes_home
from hermes_cli.config import load_env
+ config_path = get_hermes_home() / "hindsight" / "config.json"
+ config_path.parent.mkdir(parents=True, exist_ok=True)
+ config_path.write_text(
+ json.dumps({"mode": "local_embedded"}), encoding="utf-8"
+ )
+
resp = self.client.put(
"/api/memory/providers/hindsight/config?surface=declared",
json={
"values": {
- "mode": "local_external",
- "api_url": "http://localhost:8888",
- "api_key": "hs-declared-key",
+ "llm_provider": "openai_compatible",
+ "llm_base_url": "https://llm.example/v1",
+ "llm_model": "example-model",
+ "llm_api_key": "hs-llm-key",
}
},
)
assert resp.status_code == 200
assert resp.json() == {"ok": True}
- assert load_env()["HINDSIGHT_API_KEY"] == "hs-declared-key"
+ assert load_env()["HINDSIGHT_LLM_API_KEY"] == "hs-llm-key"
- config_path = get_hermes_home() / "hindsight" / "config.json"
provider_config = json.loads(config_path.read_text(encoding="utf-8"))
- assert provider_config["mode"] == "local_external"
- assert provider_config["api_url"] == "http://localhost:8888"
- assert "api_key" not in provider_config
+ assert provider_config["mode"] == "local_embedded"
+ assert provider_config["llm_provider"] == "openai_compatible"
+ assert provider_config["llm_base_url"] == "https://llm.example/v1"
+ assert provider_config["llm_model"] == "example-model"
+ assert "llm_api_key" not in provider_config
+
+ def test_declared_surface_put_keeps_local_embedded_setup_cli_owned(self):
+ resp = self.client.put(
+ "/api/memory/providers/hindsight/config?surface=declared",
+ json={"values": {"mode": "local_embedded"}},
+ )
+
+ assert resp.status_code == 400
def test_declared_surface_put_rejects_undeclared_provider(self):
resp = self.client.put(
diff --git a/tests/plugins/memory/test_hindsight_config_schema.py b/tests/plugins/memory/test_hindsight_config_schema.py
index a521ee5d2d..20793d4dbb 100644
--- a/tests/plugins/memory/test_hindsight_config_schema.py
+++ b/tests/plugins/memory/test_hindsight_config_schema.py
@@ -7,45 +7,58 @@ from plugins.memory.config_schema import (
)
+INLINE_KEYS = {"mode", "api_key", "api_url", "bank_id", "recall_budget"}
+LOCAL_EMBEDDED_KEYS = {"llm_provider", "llm_base_url", "llm_api_key", "llm_model"}
+
+
def test_hindsight_is_declared():
provider = get_provider_config_schema("hindsight")
assert provider is not None
assert provider.label == "Hindsight"
- assert {field.key for field in provider.fields} == {
- "mode",
- "api_key",
- "api_url",
- "bank_id",
- "recall_budget",
- }
+ keys = [field.key for field in provider.fields]
+ assert len(keys) == len(set(keys))
+ assert INLINE_KEYS | LOCAL_EMBEDDED_KEYS <= set(keys)
-def test_fields_are_all_inline():
+def test_local_embedded_fields_live_in_full_config():
provider = get_provider_config_schema("hindsight")
assert provider is not None
- # Hindsight is simple enough to render fully in the compact panel, so it
- # never grows a Full config… modal.
- assert all(field.inline for field in provider.fields)
+ assert {field.key for field in provider.inline_fields()} == INLINE_KEYS
+ assert LOCAL_EMBEDDED_KEYS <= {
+ field.key for field in provider.fields if not field.inline
+ }
-def test_mode_gating_is_expressed_as_select_options():
+def test_mode_write_gate_keeps_desktop_setup_to_supported_modes():
provider = get_provider_config_schema("hindsight")
assert provider is not None
mode = next(field for field in provider.fields if field.key == "mode")
assert mode.kind == KIND_SELECT
assert mode.allowed_values() == {"cloud", "local_external"}
- # local_embedded is intentionally unsupported on desktop.
+ # Desktop can display existing Local Embedded config, but setup still owns
+ # dependency installation and selecting that mode.
assert "local_embedded" not in mode.allowed_values()
-def test_api_key_is_a_secret_bound_to_env():
+def test_api_keys_are_bound_to_their_runtime_env_keys():
+ provider = get_provider_config_schema("hindsight")
+ assert provider is not None
+
+ by_key = {field.key: field for field in provider.fields}
+ assert by_key["api_key"].kind == KIND_SECRET
+ assert by_key["api_key"].env_key == "HINDSIGHT_API_KEY"
+ assert by_key["llm_api_key"].kind == KIND_SECRET
+ assert by_key["llm_api_key"].env_key == "HINDSIGHT_LLM_API_KEY"
+
+
+def test_local_embedded_llm_provider_accepts_runtime_backends():
provider = get_provider_config_schema("hindsight")
assert provider is not None
- api_key = next(field for field in provider.fields if field.key == "api_key")
- assert api_key.kind == KIND_SECRET
- assert api_key.is_secret is True
- assert api_key.env_key == "HINDSIGHT_API_KEY"
+ llm_provider = next(
+ field for field in provider.fields if field.key == "llm_provider"
+ )
+ assert "openai_compatible" in llm_provider.allowed_values()
|
|
Applied your narrower implementation. The patch resolves all 6 correctness issues:
The fix has been pushed to this branch. CI is running — will report results once available. |
|
Applied your narrower implementation. The patch resolves all 6 correctness issues:
The fix has been pushed to this branch. CI is running — will report results once available. |
|
This was generated by AI during triage. Summary: Problems:
Solution: Evidenceno deterministic fact backs this claim — model belief, not executed or read evidence Checked against |
Add llm_provider, llm_base_url, llm_api_key, and llm_model fields to the Hindsight config schema so the desktop panel can configure Local Embedded mode. Also add group annotations for better UI organization.
|
Applied the narrower fix from my review comment above. The patch:
CI passes on the schema tests (5/5). The pre-existing The contributor attribution check also failed (edrayoca+agent@gmail.com not in AUTHOR_MAP) — I've added the mapping. Ready for re-review. |
alt-glitch
added
comp/desktop
fix(hindsight): add local_embedded mode and full local-mode fields to config schema
|
|
Thanks for the review — addressing each point:
|
|
Closing: the bundled Hindsight provider has moved out of this repo, and the core-side part of this PR is already on Thanks @blut-agent for this contribution. In #119888 (merge Triage notes:
If you believe this was closed in error, comment and we will reopen. (Bulk-closed in the hindsight-move close pass.) |
7 participants
Problem
The desktop Memory & context panel for Hindsight (issue #84572) only declares
cloudandlocal_externalmodes, so Hindsight instances configured inlocal_embeddedmode display incorrect cloud defaults:local_embeddedvalue)This happens because the wizard schema (used by
hermes memory setup) declares all three modes plus local-mode fields, but the desktop'sconfig_schema.pyonly knows about cloud and local_external.Fix
Update
plugins/memory/hindsight/config_schema.pyto mirror the wizard's full schema:local_embeddedto the mode select optionswhen={"mode": "cloud"}for visibilitywhen={"mode": "local_external"}Also implement the underlying conditional visibility infrastructure:
ProviderField: newwhen: dict[str, str] | None = Noneattribute_read_field_dep(): helper to read dependency values from stored config_declared_provider_payload(): skips fields whosewhenpredicate is not metThis makes the desktop panel faithfully reflect whatever mode the wizard configured.
Test Plan